fix(launcher): reap the fingerprint records the proxy leaves behind - #345
fix(launcher): reap the fingerprint records the proxy leaves behind#345codeslake wants to merge 23 commits into
Conversation
Every proxy start writes cache-fix-proxy-<port>.sha256 into the temp dir and
nothing removed it. The port is ephemeral wherever the OS picks one, so the
records accumulate one per start without bound. runningOurCode() called that
ordinary because "systemd-tmpfiles sweeps /tmp" — a deployment assumption, not
a guarantee, and false in a container. Measured on one with zsh as PID 1 and no
sweeper: 6,743 records, 461 a day.
The cost is not the 284 KB. The launcher's own scratch-CA reaper walks
readdirSync(tmpdir()) on every start, and that scan measured 117 ms at 74,493
entries — this litter is what fills it, so the leak taxes the startup path meant
to clear it.
Seven days, matching the scratch reaper: publishFingerprint has one call site,
the spawn path, and nothing republishes, so an unrestarted holder's mtime is its
launch time. A shorter gate deletes a live holder's own record, runningOurCode()
answers null, and takeOver() exits 0 — the "swept /tmp turned a deploy into a
no-op that read as a success" incident, caused by us this time.
Driven from holdPort rather than publishFingerprint, which returns early when
the fingerprint is unreadable (reaping exactly when publishing is persistently
broken is the mistake the CA reaper documents) and runs on every respawn.
Deferred with setTimeout(...).unref() because the scan is not free and nothing
waits on it: 135-193 ms at this box's 68,533 entries. Inline it delays the bind
— interleaved against the merge base at one load, proxy-held-port.test.mjs
failed 2 of 10 runs with the scan inline and 0 of 5 with the same diff's scan
body disabled, against 0 of 14 for the base.
RECORD_PREFIX is shared by the writer and the reaper. Deriving it at runtime
read lastIndexOf("/"), which is -1 on Windows: the whole absolute path became
the prefix, no basename from readdirSync ever matched, and the reaper was a
silent no-op there.
Ref cnighswonger#304 — the record and the systemd-tmpfiles comment both arrived in 84ba7c2.
Co-Authored-By: Claude <noreply@anthropic.com>
Comments explain the code, not how it was found. The reap block carried the census that motivated it, the timings that sized it, the A/B table that placed the call site, and two quoted incidents; the test carried the same again. All of it is in 92d2c08's message and in the PR, where it is read once rather than on every visit to this file. What stays is what a reader needs in order not to break it: why seven days rather than one, why the call sits in holdPort rather than publishFingerprint, why it is deferred, and why the prefix is one shared constant. Production comment lines 36 -> 25; no behaviour change, tests unchanged and green. Co-Authored-By: Claude <noreply@anthropic.com>
Age alone made the seven-day gate a deadline rather than a margin. Nothing republishes a record — publishFingerprint has one call site, the spawn path — so a holder that neither respawns nor is redeployed for a week is fully live with an over-age record, and the next launcher to start deleted it. runningOurCode() then answers null, holderVerdict returns "holder", and takeOver() exits 0 announcing a deploy that has not taken effect: the incident the launcher already documents, made reachable by the fix meant to prevent litter. Measured end to end before this commit: a scratch TMPDIR holding an 8-day-old record for a live holder's port, one unrelated run-service start, record gone. So ask lsof, which the launcher already relies on for holderPidOn and otherHolderOn and which works on macOS. One call, made only once a record is actually eligible, so a swept host still pays nothing but a readdir. A probe that cannot answer keeps everything rather than reading "could not ask" as "nothing is listening" — that reading would hand the reaper every record on the box. The age gate stays, now bounding what a crashed holder leaves on a port nobody rebinds rather than standing alone. Two comment claims corrected. The scratch-CA reaper does not share a walk with this one: it sits after `await dispatch()` and runs in wrapper mode only, never in a run-service holder. And "age is the only discriminator available" was false — a listening-port set was one lsof away, which is what this commit uses. Tests: a live-listener case, a probe-cannot-answer case, and a case that spawns a real run-service and waits for a planted stale record to vanish. That last one exists because every other case runs lifted source and none of them can see whether the launcher ever calls the reaper — commenting the call out left them all green. Co-Authored-By: Claude <noreply@anthropic.com>
The case added to prove the reaper is reachable spawns a run-service and SIGKILLs it. run-service leaves a DETACHED standby gap-relay that stands down only for a claimant's SIGHUP, so killing the launcher reparented it to init still holding an ephemeral port. Measured: one per run, eight alive at once here, the oldest over three hours, each one a listener in the range the suite's own fixtures bind. proc-helpers exports onPort for exactly this and its comment already records the same incident from proxy-held-port. Not importing it was the whole defect. Holder first, then whatever is left on the port, which is the order that file's sweep documents. Measured after: orphan delta 0 across three consecutive runs, and no ccf-fpreap-* scratch left behind. suite-collection gains a guard for it, named rather than swept. A form-based sweep — every .test.mjs carrying both "run-service" and SIGKILL — was tried and measured: it also flags proxy-probe-bounded and stdio-epipe-survival, whose orphan delta over a full run is 0, because they spawn a holder that never reaches the standby. The predicate describes the shape of the code rather than the debt it incurs, and a guard that reds two innocent files is one someone deletes. Co-Authored-By: Claude <noreply@anthropic.com>
CI went red on Node 18 and the cause was not the Node version — it was lsof. Reproduced locally by running the file with lsof off PATH: 3 of 6 cases fail, the same failure CI reported. listeningPorts() shelled out, a missing lsof threw, and the fail-closed branch then kept every record, so the reaper became a silent no-op. The leak fix would have done nothing on any host without lsof, which is exactly where nobody would look for it. A bind answers the same question with no external tool. holderPidOn needs a PID and has to shell out; this needs one bit, and the kernel gives it directly. It is also exact where lsof was not: lsof sees only this uid, so a holder under another account read as "nothing is listening". The reap was already deferred with setTimeout and nothing awaits it, so making it async costs nothing. Every path still catches, so there is no unhandled rejection to leak out of the timer. A name whose port is not a number is kept rather than judged — listen(NaN) throws ERR_SOCKET_BAD_PORT rather than answering, and cache-fix-proxy-healthcheck.* is a name this reaper has no business deciding about. Measured: 6/6 with lsof present, 6/6 with lsof off PATH. Mutations killed — removing the port guard, and inverting the non-numeric case to reap. Co-Authored-By: Claude <noreply@anthropic.com>
The guard added alongside the orphan fix named proxy-fingerprint-reap directly, which is the same blindness the guard above it already had: it goes stale the moment a third file learns the debt. It now scans every .test.mjs that spawns a launcher and SIGKILLs it, and accepts any spelling of the sweep — onPort(), listeners(), or a raw lsof -iTCP. stdio-epipe-survival sweeps with the last of those, and a predicate that only knew the first would have called it an offender. A file that kills a launcher and genuinely owes no sweep says so with NO-STANDBY: and why. proxy-probe-bounded is the one: the hang under test is a probe, so the launcher blocks before it ever binds and there is no detached standby to reparent. Measured with its deadline forced to 200 ms, 3 s and 6 s — orphan delta 0 at all three. Measured over all 24 test files: 5 sweep, 1 exempt, 0 offenders. Mutations killed — removing the onPort sweep reds it, and removing the marker reds it naming proxy-probe-bounded. Co-Authored-By: Claude <noreply@anthropic.com>
Two claims in the reap comments were stronger than the code. It tests LISTEN, not ownership: a holder in this file's deliberate bound-but-not-listening state reads as free, which is reachable in the ~80 ms before the gap relay boots. And the loop does not yield — measured at 3,000 over-age records it held the event loop for 176 ms, because listen and close resolve on nextTick and the await never reaches the poll phase. Neither changes what the code should do. Both would have been inherited as fact. Co-Authored-By: Claude <noreply@anthropic.com>
Three claims were stronger than what had been measured, all in permanent records. 4411af4's message said "measured over all 24 test files: 5 sweep, 1 exempt, 0 offenders". That number came from a review summary rather than from running the predicate. Run here over the guard's own testDir: 114 .test.mjs, 7 candidates, 6 sweep, 1 exempt, 0 offenders. The guard was right; the census reporting it was not, and it omitted proxy-fingerprint-reap itself. The "~80 ms before the gap relay boots" was borrowed from a nearby comment that measured the proxy CHILD's boot, a much larger spawn. Nothing measured the relay, so the comment now says the window is unmeasured rather than naming a number. And portFree is itself a listener while it asks. A launcher starting concurrently runs otherHolderOn(), which selects on a LISTEN socket plus a run-service command line plus greater uptime; a peer mid-probe can satisfy all three and be read as an incumbent. Bounded by the bind lifetime, under 59 µs per record, and now stated. Co-Authored-By: Claude <noreply@anthropic.com>
The predicate accepted `-iTCP` as a spelling of "sweeps the port", and stdio-epipe-survival passed on it. That file's `lsof -iTCP` is its STIMULUS — it kills the proxy child mid-body to provoke a restart log, three lines above the assertion — while its actual cleanup is `t.after(() => reap(holder))`, a process-GROUP kill. The file was protected the whole time, by a mechanism the guard could not see, and passed on a coincidental substring. A group kill reaps the standby as a child of the group without ever naming a port, so it discharges the same debt. The predicate now names that instead of a third spelling of lsof, and the comment says which three mechanisms count and why they look nothing alike. Measured: all 24 candidates reclassify identically (6 sweep, 1 exempt, 0 offenders), and removing the group kill from stdio-epipe-survival now reds the guard naming that file. Co-Authored-By: Claude <noreply@anthropic.com>
… code
CI went red on Node 22 with `body.startsWith is not a function`, inside "refuses
nothing when the proxy under it dies". Six of this file's seven probes resolve
`ERR:${e.code}` — a string with the prefix classify() tests for. The seventh, at
the forced-kill case, resolves a bare `r.statusCode` on success and a bare
`e.code` on error. Its caller filters out 200 and hands everything else to
classify(), so a 502 arrives as a Number and the case dies where the answer is
simply "that was a reply, not an outage".
The path only opens when a non-200 is actually observed, which is why it survived
every local run and three CI matrices before this one. A unit case pins it now:
a status code classifies as null, an ERR: string still classifies, and a bare
ECONNRESET with no prefix stays null.
Measured: removing the type guard reds that case with the exact CI message.
Nothing else on this branch touches this file — the crash predates it and was
merely surfaced here. It rides along rather than waiting behind its own PR
because leaving this branch red would mean explaining the red in a comment and
making the two land in a fixed order, for a five-line guard.
Co-Authored-By: Claude <noreply@anthropic.com>
A hardcoded port sits inside the kernel's ephemeral range, so a sibling test's launcher can be handed it and hold it for a whole run. portFree then answers false, the reaper correctly keeps the record, and only this file is wrong — a red that reads as flake. Measured: occupying 40404 turns "a record whose port still has a listener is kept however old it is" red on demand and leaving it free turns it green; occupying 40808 exhausts the e2e case's 25s deadline. Across ten interleaved suite runs these were the only two failures, and they repeated rather than wandered, which is what separates them from the load-shaped reds this suite also has. The three cases whose records must actually be reaped now take a port the kernel has just released. The four fixtures that never reach portFree — kept by the age gate or the suffix filter — move below the ephemeral floor so they cannot collide with the derived one. The e2e case reuses the same helper instead of its own inline copy. Mutation table unchanged: forcing portFree true still kills two cases, removing the age gate still kills one. Co-Authored-By: Claude <noreply@anthropic.com>
|
Field data from running this branch, in case it is useful for review. Scale of the leak this addresses. Three consecutive full-suite runs on a tree carrying neither this PR nor #347 left 102 orphaned fingerprint records in the shared temp directory — one per launcher spawned, all on ephemeral ports with no surviving process. A tree carrying both left zero there. Attribution, stated honestly. That zero is primarily explained by #347's per-file On the guard that matters. Reading the reaper rather than measuring it: it skips a record whose port is not free ( The 7-day age bound on top reads right as well — it bounds what a crashed process can leave behind without racing anything that is still coming up. 🤖 Generated with Claude Code — Proxy Builder |
classify() reads nothing but an ERR: prefix. The probe in "refuses nothing when the proxy under it dies" resolved a bare statusCode on success and a bare e.code on error, so every value it handed classify() came back null and the refusal count derived from it was empty whatever the holder did. The bound that count asserts could not fail. Measured against the shipped classify(), a probe run at a port with no listener: null, where the case needs "refused". That probe is hoisted to module scope so a case can exercise it, and now resolves what the file's six other probes resolve: 200, or an ERR: string carrying the status and the body. The new case aims at port 1, which needs root to bind, so it allocates no ephemeral port that the OS could hand to a launcher about to bind one. The readiness assertion in withHeldPort carried the same defect in the other direction: a 200 from something that is not our proxy died as a bare SyntaxError with the body discarded, so a red run named no responder. It now reports what answered, which is the rule the probes in this file already follow. Co-Authored-By: Claude <noreply@anthropic.com>
…shared hop The spawn case set CACHE_FIX_FORWARD_PROXY=on and pinned TMPDIR but not CLAUDE_CONFIG_DIR, so the launcher minted a CA in the operator's real config dir and republished ca-trust.d/ccf.pem, the rendezvous file every sibling component reads, from under whatever proxy was serving. Measured with the config dir redirected to a probe: it holds ca-trust.d and cache-fix-ca after the case, and stays empty without the variable. The reap needs no CA, so the variable goes and the config dir joins TMPDIR in the scratch dir the case already removes. The hop scrub was a hand-written four-name list standing beside the shared nine-name one this file already imports from. It left CACHE_FIX_UPSTREAM_PROXY and CACHE_FIX_FALLBACK_PROXIES, the two the relay reads first, in the child. releasedPort() re-implemented the shared freePort() byte for byte, and the presence assertion on recordAgeMs guarded nothing the age cases do not already cover: with the gate disabled the stale-record case fails on its own. Co-Authored-By: Claude <noreply@anthropic.com>
54 of the 83 lines this change added to the launcher were comment, and most of the surplus was one run's numbers: microseconds per bind, milliseconds held per thousand records, a sizing budget for a scan nothing waits on. They date the file without telling a reader anything the code does not. Kept: why a listening port outranks the clock, what losing a live holder's record costs, and both directions of the gap between "is anything listening" and "is anyone using this port". Co-Authored-By: Claude <noreply@anthropic.com>
The classify() guard and the unit case below it carried the same six-line account of the seventh probe; one copy is the record, two is drift waiting to happen. portFree's third paragraph restated the deferral its call site already states, and the yield invariant it also carried is kept. The non-numeric-port case built a whole scratch dir to assert one filename survives. The multi-name case already builds one and runs the same reaper, so the name joins it there, over-age so it still reaches portFree. net: -18 lines. No behaviour change; 22/22 pass on the two files that hold the reaper's tests and the suite-wide guards. Co-Authored-By: Claude <noreply@anthropic.com>
Three full-suite runs went red in this file at three different cases, about
two seconds in, two of them on JSON.parse at position 3. Nobody had named
what answers 200 with such a body. It is us.
takePort() binds 0, reads the port and RELEASES it before the launcher binds
it, and every worker draws from one ephemeral pool: measured, 1317 of 4000
ports drawn by that allocator were handed to a concurrently running process
in the same window. Two files stand up a stand-in release channel on
127.0.0.1:0 that answers ANY path with 200 and a bare version string, and
`JSON.parse("2.1.222")` dies at exactly position 3 — a three-character JSON
number followed by a dot. A census of every literal HTTP body in test/,
proxy/ and bin/ finds no other responder in the tree that lands there.
So the readiness rule was the defect, not the launcher: four loops here broke
out on the first 200 and handed it to a bare JSON.parse, which reds the
holder's case for a neighbour's fixture. readyBody() is the one test for "the
holder is up", the loops poll until it or the deadline, and the last body
still rides along on a real timeout.
Also: the stand-in readiness budget in the forced-kill case was 10s, the
outlier in a file where every other launcher-spawned child gets 15s or 20s.
It expired under load on a case that costs 3.3s quiet.
37/37 pass in this file. Mutation-checked: with the predicate back to
!startsWith("ERR:"), the new case reds as "a release channel's 200 read as
readiness", actual true, expected false.
Co-Authored-By: Claude <noreply@anthropic.com>
Review findings, each mutation-checked against the final code. RECORD_PREFIX existed so the writer and the reaper could not drift apart. The suffix could still drift: fingerprintPath built ".sha256" in a template while the reaper matched and sliced three separate literals. Nothing caught it either, because the fixtures hardcoded the suffix on both sides, so a change at the writer left the reaper matching nothing, collecting nothing, and every case green. RECORD_SUFFIX now, derived by the test from the launcher's own source. Mutation: ".sha256" to ".fp" reds 3 of 5 cases. The scan was deferred so it would not delay the bind, and then held the event loop anyway: nothing it awaits reaches the poll phase, so an uninterrupted pass blocks between the bind and the first accept. One periodic setImmediate. Measured at 300 records: a setImmediate queued first fires 13 ms into a 28 ms scan with the yield and not at all without it, and the total is unchanged. The seven-day gate was two independent literals a thousand lines apart, one of them justified by a comment saying it matches the other. One REAP_AGE_MS, read by both reapers. Two gaps the probe has and did not name: a record is keyed by port alone while portFree asks about bindAddr(), so a holder on another address reads as free; and the probe is itself a listener, so a launcher in otherHolderOn() can call it an incumbent and settle(0) on an empty port. Both now stand beside the LISTEN-vs-ownership gap, with the shared consequence stated once. Not closed here: the wildcard probe that would fix the first opens an externally reachable socket per over-age record, and carrying the address would change the record format. The port floor is load-bearing and now has a case: listen(0) takes a random free port and always succeeds, so without it every port-0 record reads as collectable, and port 0 is a name older versions demonstrably wrote. The CEILING is invisible from the directory — listen() throws, the inner catch keeps the record, and a fixture asserting the record survived passes with the guard deleted. Measured, then replaced with a direct question to the predicate, where answering false and rejecting differ. 5/5 in the reap file, 37/37 in held-port, 22/22 across the guards. Co-Authored-By: Claude <noreply@anthropic.com>
rec() was added with the constant-derivation change and never wired to a call site. A one-use local alias for REAP_AGE_MS stood in front of the constant it aliases. And a comment above the deferral assertion restated the assertion's own message, which is the half that reaches a failing run. net: -4 lines. 23/23 across the reap file and the suite-wide guards. Co-Authored-By: Claude <noreply@anthropic.com>
…l it The reaper e2e case initialises `port = 0` and assigns it from freePort() seven lines later; its finally sweeps onPort(port) unconditionally. Anything that throws in between aims that sweep at port 0. ours() reads CACHE_FIX_PROXY_PORT, and a proxy child legitimately carries 0 there when it inherits the holder's listening fd. So onPort(0) is not the empty set it looks like. Measured read-only on a dev host: 7 processes, one of them the deployed proxy every session on the box routes through. Guarded at onPort() rather than the call site -- three other files sweep through it and none of them state the invariant either. listeners(0) is already empty (nothing listens on port 0), so the whole hole is ours(). The new case carries its own control: it asserts the helper DOES find a stub on a real port before asserting it does not find one on 0. Without that, a host with no proxy running passes it vacuously. RED: onPort(0) selected a process carrying CACHE_FIX_PROXY_PORT=0 GREEN: 6/6 in the file, 104/104 across every file that calls onPort Co-Authored-By: Claude <noreply@anthropic.com>
publishFingerprint writes `<record>.<pid>` and renames. The suffix test skips that name, so a publish that died between the write and the rename leaves a file no reaper anywhere ever collects -- and the case in this file pinned that, asserting an EIGHT-DAY-OLD temp must survive as "a concurrent launcher's pending write". A rename pends for microseconds. Past the same seven-day gate the only thing that leaves one behind is a crash, and the scratch-CA reaper twenty lines below already answers this exact question for its own artifacts rather than sparing them forever. The protection is kept and split from the leak: a FRESH temp still survives untouched, an over-age one is collected. A temp answers to no port, so it skips portFree -- nothing reads it. RED: an eight-day-old <record>.<pid> survived: no reaper anywhere collects it GREEN: 6/6 in the file Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Codex review:
Review: PR #345 launcher fingerprint reap
Date: 2026-08-26
Reviewed: bin/claude-via-proxy.mjs, test/proxy-fingerprint-reap.test.mjs, and adjacent launcher test harness changes at 6f695ace44451c3c1404135db77e0c59ca524aac
Round: 1
Label applied: changes-requested
What Is Correct
- Measured:
node --test test/proxy-fingerprint-reap.test.mjson node v24.11.1 passed 6/6 in 542 ms. The new tests cover the happy path for a stale record on an unused port, keeping fresh records, keeping records whose port is listening, keeping fresh in-flight temp records, removing abandoned temp records, and driving the reaper from a real launcher startup. - Measured:
npm_config_cache=/tmp/npm-cache-pr345 npx --yes node@18 --test test/proxy-fingerprint-reap.test.mjson node v18.20.8 passed 6/6 in 552 ms, so the focused file passes at the package floor runtime I could run locally. - Read: the reaper is best-effort and crash-local. It only removes individual matching files under
tmpdir()and catches per-entry failures, so a crash or refused delete leaves survivors as stale disk state rather than corrupting a shared manifest (bin/claude-via-proxy.mjs:796,bin/claude-via-proxy.mjs:817). - Reported from GitHub status rollup on PR head
6f695ace: Test matrixtest (18),test (20), andtest (22)all completed successfully, as did GitGuardian and Snyk.
Blockers
-
The reaper can delete a fresh fingerprint that replaces the stale one after the stale stat.
Evidence: Measured. I lifted the PR's
reapFingerprintRecords()andportFree()frombin/claude-via-proxy.mjs, created an over-agecache-fix-proxy-<port>.sha256, and made thestatSync()wrapper perform the same temp-write +renameSync()publish thatpublishFingerprint()uses before returning the stale stat. Result:{"publishedFresh":true,"existsAfter":false,"contentAfter":null}Evidence: Read. The production sequence is
statSync(p)-> age check ->await portFree(...)->rmSync(p)with no second stat, generation check, rename-to-quarantine, or other compare step before unlink (bin/claude-via-proxy.mjs:812,bin/claude-via-proxy.mjs:815,bin/claude-via-proxy.mjs:816). A launcher publishes by writing<record>.<pid>and renaming it over the record path (bin/claude-via-proxy.mjs:776,bin/claude-via-proxy.mjs:778), and the holder writes that record before every child spawn (bin/claude-via-proxy.mjs:1260,bin/claude-via-proxy.mjs:1287). There is no heartbeat; the comment says it is written on spawn/restart, not periodically (bin/claude-via-proxy.mjs:1276).The blast radius is load-bearing. If the fresh record is removed,
runningOurCode()returnsnullfor the port (bin/claude-via-proxy.mjs:859,bin/claude-via-proxy.mjs:861),holderVerdict()treats unknown as a holder of ours (bin/claude-via-proxy.mjs:520,bin/claude-via-proxy.mjs:523), andtakeOver()exits without replacing an incumbent holder when it cannot identify one (bin/claude-via-proxy.mjs:1687,bin/claude-via-proxy.mjs:1690). That is silent ownership drift except for the warning path, not self-healing. Please make the remove conditional on the record still being stale afterportFree()returns, and add an adversarial test for stale-stat/fresh-rename/unlink. A second stat immediately before remove that skips if the path has become fresh would address the measured race; account for filesystem timestamp granularity in the test/oracle.
What Needs Attention
- Measured: I started a broader adjacent run with
node --test test/proxy-fingerprint-reap.test.mjs test/proxy-held-port.test.mjs test/suite-collection.test.mjs test/proxy-probe-bounded.test.mjson node v24.11.1. The new fingerprint file passed first, but the held-port suite then reported multiple failures and the combined process was still running after more than two minutes, so I stopped it. I am not treating that as a PR regression because the GitHub matrix is green and the focused file passed locally on v24 and v18, but the local held-port result is not clean evidence for approval. - Read: the concurrent-write test coverage currently protects a fresh
<record>.<pid>temp file (test/proxy-fingerprint-reap.test.mjs:208,test/proxy-fingerprint-reap.test.mjs:243), but it does not cover the more dangerous interleaving where that temp is renamed over an already-classified stale record beforermSync().
Bloat / Non-Functional
- Metrics: production diff is
bin/claude-via-proxy.mjs+92/-4; test/support diff is +405/-25, for about 4.4 test added lines per production added line. New files: one test file. New exports: 0. New env vars: 0. New on-disk path shapes: no new canonical record path; it also starts collecting abandoned<record>.<pid>temp paths. Added production comment/blank-to-code ratio inbin/claude-via-proxy.mjsis about 55:33. Given the load-bearing ownership predicate and the repo's existing style, I do not see actionable bloat. - The PR does not include a
## Non-Functional Requirementssection. At 92 production added lines this is below the repo's rough 300-prod-LOC community-PR threshold, so I am not blocking on that. Independently, the reap predicate is load-bearing because it decides whether launcher ownership state is valid.
Recommendations
- Add a regression test that forces the stale stat, publishes a fresh record at the same basename before removal, and asserts the fresh record survives. This should run against the shipped decision path or a lifted slice with controls proving it reaches the same
stat -> portFree -> rmsequence. - Document the intended concurrency contract explicitly. The implementation currently behaves as “reap wins,” but there is no heartbeat to re-establish a lost fresh record, so the safe contract needs to be “fresh publish wins after classification” or an equivalent recovery mechanism.
Bottom Line
Request changes. The happy-path reap and basic stale/fresh predicate are covered and pass locally, but the load-bearing concurrent publish race is real and measured: the reaper can unlink a fresh fingerprint after examining an old one, and the launcher does not periodically self-heal that loss.
— Codex, cross-LLM review, round 1
…own probe The decision sequence is stat -> age check -> AWAIT portFree() -> rm, and that await is a real gap. `publishFingerprint` publishes by renaming `<record>.<pid>` over the record path, so another launcher can replace the file between the classification and the unlink -- and the unlink then acts on a judgement the file no longer answers to. Losing a FRESH record is not disk litter. `runningOurCode()` answers null for that port, `holderVerdict()` reads unknown as an incumbent of ours, and `takeOver()` exits 0 printing that the deploy has NOT taken effect while every other check on the host stays green. Re-read after the probe and skip if the path is no longer over-age. The remaining window is two syscalls with nothing awaited between them, against the whole bind-and-close of the port probe before. RED: the reaper unlinked a record republished during the port probe GREEN: 7/7 in the file mutation: dropping the re-read kills exactly the new case; restored 7/7 Co-Authored-By: Claude <noreply@anthropic.com>
Resolved test/proxy-held-port.test.mjs: both sides add the same type guard to classify() and differ only in wording. Kept cnighswonger#355's, because cnighswonger#345's says "health() replaced the probe that resolved a bare statusCode" and health() itself resolves a bare 200 -- refuted in review. Behaviour is identical. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — the race is real and it is fixed, at a head later than the one reviewed. Reviewed: The fix. if (Date.now() - statSync(p).mtimeMs <= REAP_AGE_MS) continue;
if (isRecord && !(await portFree(...))) continue;
// RE-READ: publishFingerprint renames a new record over this path, and the
// probe's await is wide enough to land inside.
if (Date.now() - statSync(p).mtimeMs <= REAP_AGE_MS) continue;
rmSync(p);The adversarial test you asked for is on the branch as Your blast-radius reading matches ours and is why the re-stat is a Filesystem timestamp granularity is handled by the test driving the publish Could you re-review at 🤖 Generated with Claude Code |
…ger#356 merge left behind conflict-shape.sh's additive resolution on the cnighswonger#356 merge kept both sides of two hunks where cnighswonger#356 (cut from upstream, before cnighswonger#345 or cnighswonger#355 existed) independently re-added content cnighswonger#345 had already added on this branch: a second `if (typeof body !== "string") return null;` guard (with its own, now-superseded comment) stacked dead beneath the one already in classify(), and a second, byte-identical copy of the "classify survives a probe that answers with a status code" test right after it. Both were textually different insertions at the same conflict hunk (so "additive" concatenated them) but semantically the same content twice. Neither duplicate changed behaviour -- the second guard clause is unreachable, and node:test does not refuse a duplicate case name -- so the suite passed either way; kept once, as the recorded resolution for this file already documents keeping ours' guard. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFPGPPSYmx8NNGqbEsSpNc Co-Authored-By: Claude <noreply@anthropic.com>
Conflict in test/proxy-held-port.test.mjs (classify()'s typeof guard): matches rebuild-resolutions.md's alternative 2, block 1, standalone (block 2 of that entry does not arise in this build order since cnighswonger#345 has not merged yet). Kept ours' guard (if (typeof body !== "string") return null;, carried into this build via cnighswonger#356's cherry-pick of cnighswonger#345's 35ac847) and dropped theirs' (cnighswonger#355, 5d2159f) removal of it. Measured: re-applying theirs' hunk alone and running 'classify survives a probe that answers with a status code' reproduces the exact TypeError the ledger records (body.startsWith is not a function). The resolved file passes that case and the other two classify cases (3/3); the parent commit passes them too, so nothing here is new. Co-Authored-By: Claude <noreply@anthropic.com>
…to HEAD Two judgement conflicts, both orthogonal-halves-of-one-block (same pattern already recorded for cnighswonger#368/cnighswonger#369): test/proc-helpers.mjs: cnighswonger#345 guards onPort(0) against selecting every proxy child (CACHE_FIX_PROXY_PORT=0), cnighswonger#369 (already in this build) added probeHealth/waitForHolder right after the same line. No shared subject; kept both — theirs' guarded onPort(), ours' probeHealth/waitForHolder unchanged. test/proxy-held-port.test.mjs, block in 'refuses nothing when the proxy under it dies': cnighswonger#345 proposes swapping the local 'ok'-sentinel probe (cnighswonger#355's, ours) for the shared health(port) helper it adds elsewhere in the file, which resolves the number 200 rather than the string "ok". Three lines below this hunk, 'const cut = seen.filter((c) => c !== "ok")' already depends on the 'ok' sentinel — taking theirs would silently make cut === seen (the exact defect this file's classify()/probe rewrite exists to prevent). Kept ours whole. Also dropped two merge-additive duplicates the mechanical classifier cannot see, same class as the recorded 'cnighswonger#356 duplicates' ledger entry: a byte-identical second copy of 'classify survives a probe that answers with a status code' (cnighswonger#345's own commit landing both directly and via cnighswonger#356's earlier cherry-pick of it), and a second, differently-worded typeof guard cnighswonger#345 stacked under the first. Verified: node --test test/proxy-fingerprint-reap.test.mjs (7/7, including 'onPort(0) selects nothing, and still selects on a real port'), and --test-name-pattern=classify in test/proxy-held-port.test.mjs (4/4, no duplicate case). Co-Authored-By: Claude <noreply@anthropic.com>
…er#370's guard requires This file (from cnighswonger#345, upstream has no such file) spawns the launcher but never called armLineage()/reapStamped(), so cnighswonger#370's suite-collection.test.mjs guard 'every test file that spawns the launcher or relay carries a lineage marker' flagged it in the merged build. Armed the same three-line way cnighswonger#370 armed its other eight files: import armLineage/reapStamped, armLineage() at module scope, reapStamped() in a file-level after(). Co-Authored-By: Claude <noreply@anthropic.com>
The record nobody collects
publishFingerprintwrites<tmpdir>/cache-fix-proxy-<port>.sha256once, on the holder's spawn path. Nothing republishes it and nothing removes it.runningOurCode()reads it to decide whether an arriving launcher is looking at the same build — so a missing record makes that answernull,holderVerdict()reads unknown as an incumbent of ours, andtakeOver()exits 0 while printing "if this was a deploy, it has NOT taken effect." Every other check on the host stays green through that.The launcher now reaps them on the way up, off the startup path:
setTimeout(reapFingerprintRecords, 0).unref()fromholdPort, once per launcher process rather than once per spawn.What it removes, and what it will not touch
A record goes only when it is over the age gate and its port does not answer. Both conditions, in that order.
A temp —
<record>.<pid>, whatpublishFingerprintwrites before its rename — is different, and this is the one asymmetry worth stating plainly: a temp answers to no port, so it skips the probe entirely and the age gate is its only discriminator. A pending rename lives for microseconds; past the gate the only thing that leaves one behind is a publish that died. The suffix test used to skip that name forever, and a case here pinned that, asserting an eight-day-old temp must survive.The age gate is
REAP_AGE_MS, shared by construction with the scratch-CA reaper twenty lines below rather than by a comment claiming they match.Three things the port probe does not answer
portFree()asks the kernel to bind, because that needs one bit and a host withoutlsofwould otherwise make the reap a silent no-op.bindAddr(). A holder on another address reads as free to a reaper on loopback.otherHolderOn()can read it as an incumbent.All three end in the same place:
runningOurCode()answers null and the deploy announces itself as not taken effect. The local variant of (3) cannot happen —listen()is the last synchronous statement ofholdPort's executor, so this launcher already owns its own port when the reap runs.The scan yields
readdirSync(tmpdir())on a developer box here: 17,905 entries in 33.7 ms. The loop hands the event loop back every 100 entries, because nothing else awaited in it reaches the poll phase and an uninterrupted pass would hold the loop between the bind and the first accept — the delay deferring the reap was meant to avoid.Two defects this branch fixed in itself
onPort(0)selected the live proxy. The e2e case initialisesport = 0and assigns it fromfreePort()seven lines later; itsfinallysweptonPort(port)unconditionally, so anything throwing in between aimed a SIGKILL sweep at port 0.ours()matchesCACHE_FIX_PROXY_PORT, which a proxy child legitimately carries as0when it inherits the holder's listening fd — so port 0 is not the empty set it looks like. Measured read-only on a dev host: 8 processes, one of them the deployed proxy serving 9901. Guarded atonPort(), where three other files' sweeps also route.The reaper could unlink a record published inside its own probe.
stat→ age check →await portFree()→rm, and that await is a real gap: another launcher publishes by renaming over the same path. Re-read after the probe. Measured: the closed window is 41.8 µs median, the residual (two syscalls, nothing awaited between them) 3.0 µs.Verification
Focused file 7/7; whole suite 1957 pass / 0 fail / 1 skipped (the skip is pre-existing and unconditional). CI green on node 18 / 20 / 22.
Number(port) > 0inonPortonPort(0) selected a process carrying CACHE_FIX_PROXY_PORT=0; the sweep would SIGKILL the live proxystatSyncthe reaper unlinked a record republished during the port probeendsWithan eight-day-old <record>.<pid> survived: no reaper anywhere collects itn < 1port floorport 0 was probed6 files changed, +540 / −29. Production is
bin/claude-via-proxy.mjs+97 / −4 — 34 code, 59 comment, 4 blank, a 1.74:1 comment ratio. Tests are +443 / −25. New files: 1. New exports, env vars, on-disk path shapes: 0.Known, and not fixed here
setImmediateyield is pinned only by a source regex; no case executes it, because the largest directory any reaper run scans in the suite is 9 entries. A positive control at 301 entries fires it three times in 4 ms.